feat(diag): show applied and pending firewall rules - #53
Conversation
b000516 to
0dd295f
Compare
Three sources, because they answer three different questions and are not interchangeable. What dezhban recorded installing. internal/applied writes the exact ruleset text handed to the backend, timestamped, beside state.json at 0644 like the state file — so the unprivileged menubar app can read it. Recorded by wrapping the runner's Backend rather than by calling Save at each Apply: the run loop applies from nineteen places, and a record only as complete as the last person to remember it is worse than none. The wrapper adds no goroutine and no writer, so the single-writer invariant is untouched, and it records only after a successful Apply — a failed one leaves the previous ruleset live, and describing rules that were never installed is the one thing a reader of this file must be able to rely on not happening. Unblock and Cleanup clear it, so a stale ruleset can never be read as the live posture. What the kernel holds. FirewallBackend gains InstalledRules, implemented for pf, nft and WFP, each scoped to dezhban's own anchor/table/group so it can never become a way to dump unrelated firewall state. It is a read: it does not go through Apply and does not touch the single-writer rule. It needs root, which is why nothing calls it on a tick. pf and nft additionally flag the loaded-but-not- evaluated cases their IsBlocked already checks for. What each posture would apply, which print-rules already rendered purely. A record with no kernel rules is reported and never repaired — the run loop's verify tick already owns that, and a second repairer would be a second writer. Neither surface diffs the two texts: the kernel renders its own normalised form of what was loaded, so a byte comparison would report drift on every healthy host. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0dd295f to
9167eaf
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Several failure and teardown paths can leave diagnostics stale or incorrectly report the firewall’s actual state.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds firewall-rule diagnostics across the daemon, CLI, platform backends, and macOS app.
Changes:
- Records successfully applied rulesets.
- Adds applied, installed, and preview rule views.
- Documents and tests the diagnostic workflow.
File summaries
| File | Description |
|---|---|
CHANGELOG.md |
Records the feature. |
cmd/dezhban/main.go |
Adds CLI flags and output. |
docs/concepts/modes.md |
Explains rule sources. |
docs/contribute/testing.md |
Adds on-host checks. |
docs/usage/cli.md |
Documents CLI usage. |
gui/macos/Sources/DezhbanCore/Rulesets.swift |
Defines diagnostic models. |
gui/macos/Sources/DezhbanMenu/AppState.swift |
Manages rule reads. |
gui/macos/Sources/DezhbanMenu/DezhbanCLI.swift |
Invokes CLI diagnostics. |
gui/macos/Sources/DezhbanMenu/DiagnosticsView.swift |
Renders firewall diagnostics. |
gui/macos/Tests/DezhbanCoreTests/RulesetsTests.swift |
Tests model decoding. |
internal/applied/applied.go |
Persists applied rules. |
internal/applied/applied_test.go |
Tests persistence behavior. |
internal/firewall/backend.go |
Adds installed-rule readback. |
internal/firewall/nft_linux.go |
Implements nft readback. |
internal/firewall/pf_darwin.go |
Implements pf readback. |
internal/firewall/render_darwin.go |
Names pf rulesets. |
internal/firewall/render_linux.go |
Names nft rulesets. |
internal/firewall/render_windows.go |
Names WFP rulesets. |
internal/firewall/wfp_windows.go |
Implements Windows readback. |
internal/runner/recording.go |
Wraps backend recording. |
internal/runner/recording_test.go |
Tests recording behavior. |
internal/runner/runner.go |
Configures recording path. |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| PollCommand: pollCommand, | ||
| Publish: publish, | ||
| BlockedCountries: cfg.BlockedCountries, | ||
| AppliedRulesPath: applied.Path(stateDir()), |
| "$g = Get-NetFirewallRule -Group " + groupName + " -ErrorAction SilentlyContinue", | ||
| "if ($null -eq $g) { 'NONE'; exit 0 }", | ||
| "'# default outbound action per profile'", |
| if err := r.Backend.Apply(p); err != nil { | ||
| return err |
| static func readAppliedRules() -> AppliedRuleset? { | ||
| guard let bin = binaryPath() else { return nil } | ||
| let r = exec(bin, ["print-rules", "--applied", "--json"]) | ||
| guard r.status == 0, let data = r.out.data(using: .utf8) else { return nil } | ||
| return AppliedRuleset.decode(data) |
| if main, err := pfctlCtx(ctx, "", "-s", "rules"); err == nil { | ||
| if mainRulesetReferencesAnchor(main) { | ||
| b0.WriteString("# main ruleset references the dezhban anchor\n") | ||
| } else { | ||
| b0.WriteString("# WARNING: the main ruleset does NOT reference the dezhban anchor —\n") | ||
| b0.WriteString("# these rules are loaded but pf never descends into them.\n") | ||
| } | ||
| } |
Found reading the diff before review, not by the reviewer. print-rules now carries two kinds of flag: one describing a ruleset to RENDER (--mode) and two selecting a live ruleset to REPORT (--applied, --installed). Only the --applied/--installed pair was refused. The other two combinations were accepted and half-discarded: print-rules --applied --mode fullblock # --mode ignored, exit 0 print-rules --json # --json ignored, prints text Both are the shape this project calls its worst bug — a flag accepted and then quietly dropped — and the second is the more misleading, since a caller parsing that output gets firewall syntax where it asked for JSON. Each is now refused with exit 2 naming the flag to drop, matching the existing --applied/--installed refusal. The check is on what the user TYPED, via fs.Visit, not on flag values: --mode defaults to "guard", so testing its value would reject every plain --applied run. TestPrintRulesAppliedIsFineWithoutMode pins that, and TestPrintRulesRefusesFlagsItCannotHonour covers all four refusals — the two new cases return 0 and 1 on the unfixed code. Also: CLAUDE.md said the privileged set was "exactly" a list that did not include this, and listed print-rules among the commands needing no root. `--installed` reads the kernel back and does need it. Corrected using the same "X but not X --sub" idiom the paragraph already uses for `setup` and `vpn list`, and the refusals are documented in cli.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 1 of the review loop, from two independent reviewers (GitHub Copilot on the PR and a read-only local agent) that agreed on the headline finding. **`panic` and `unblock` never cleared the record.** Both tear rules down through a raw `firewall.New()` backend, and only the runner's decorator knew how to clear `applied-rules.json`. So after `sudo dezhban panic` the record survived, and `print-rules --applied` and the Diagnostics pane both went on reporting "guard applied at 14:02" over a network that command had just thrown wide open. `panic` is the worst place for this: it is deliberately independent of the running service, so the deferred Cleanup that normally clears the record never runs, and it is the moment an operator is asking precisely whether the rules are gone. Both paths now clear it, and clear it even when the teardown reported an error — the rules are then in an unknown state, and a record that confidently names the old posture is worse than none. **`block` recorded nothing.** The mirror of the same gap: rules installed by hand were absent from a diagnostic that claimed to show what dezhban had applied. It understates rather than overstates, but a record that is only truthful when the service happened to be enforcing is not one an operator can use. Also from the same round: - Windows reported "no dezhban rules are loaded" for a host that is fully cut. `Remove-NetFirewallRule -Group dezhban` takes away only the allow rules, so a profile whose `DefaultOutboundAction` is still `Block` is enforcing with no group present — and the readback returned before ever emitting the profile table. The defaults are now read first and unconditionally, and the CLI prints the text even when no group is loaded. The "no rules" answer is also found as its own line rather than by matching the whole output, since `-ErrorAction SilentlyContinue` leaves warnings on the success stream — incidental text made "no rules" read as "rules loaded" and displayed the noise as the kernel's ruleset. - pf dropped its anchor-reference verdict silently when the main-ruleset read failed, so "could not check" rendered identically to "checked, fine" — for the exact non-enforcing state that check exists to expose. It now says so, and gets its own timeout instead of the remainder of the anchor read's. - `--installed --json` printed a stderr note on a corrupt record. The app captures stdout and stderr together, so that prepended prose to the document and turned a good privileged readback into an error in the pane. Human output only now. - A corrupt record made `--applied` exit 1, contradicting internal/applied's own "discarded, never fatal" contract. It is now reported on stderr and treated as absence, which is what the contract says and what the test name already claimed. - `--config` was accepted and discarded on `--applied`/`--installed` — the same shape 48340c2 refused for `--mode`, one flag over. - The kernel readback is a snapshot nothing refreshes, shown under "In the kernel now". Read during GUARD, it kept describing the firewall after FULL BLOCK engaged. It is now titled with the time it was read, cleared when the pane refreshes or closes, and its caption says it is a snapshot. Timestamps also carry their date: a record from three days ago rendered as a bare "14:02:11" reads as today. - Two on-host checks asserted things a correct build fails: that `panic` cleared the record (it did not, until now), and that a collapsed Diagnostics pane spawns no `print-rules` — it always fetches the cheap `--applied --json` row. Both reworded, and the teardown check now names all three routes separately. - CLAUDE.md: 48340c2 put `print-rules --installed` in the privileged set, but that set auto-re-execs under sudo via requireRoot and this deliberately does not — nor does it need elevation on Windows. Now described as what it is, outside the set. Tests: teardown-clears and block-records (both fail on the unfixed code), the corrupt-record exit status, the `--config` refusal, the Windows marker scan against captured output, and the nanosecond timestamp form `time.Now()` actually emits — the fixtures only covered six digits. The record path is now injectable so these do not read the developer's own live ruleset off /var/db/dezhban. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Stale records, asynchronous invalidation, malformed JSON handling, and Windows enforcement-state reporting can produce misleading diagnostics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
gui/macos/Sources/DezhbanCore/Rulesets.swift:93
- Defaulting every missing or mistyped field means any JSON object—including
{}—decodes successfully as “no rules loaded,” causing Diagnostics to present a benign standby message for a malformed or incompatible CLI response. Treat the required fields as required so invalid output follows the existing error path instead of becoming false reassurance.
internal/runner/recording.go:79 - If this atomic save fails, the previous record remains on disk even though a different policy was successfully applied. Diagnostics will then present the old posture as the applied one; this conflicts with the wrapper's own rule that stale data is worse than no record. Clear the previous record on the save-failure path, as the WFP applied-action recorder already does.
- Files reviewed: 25/25 changed files
- Comments generated: 4
- Review effort level: Balanced
| // Text is returned either way: with no group there is still a profile table | ||
| // worth reading, and it is the half that says whether egress is cut. | ||
| return out, !hasNoRulesMarker(out), nil |
| if err := applied.Save(appliedPath(), rec); err != nil { | ||
| fmt.Fprintln(os.Stderr, "warning — could not record the applied ruleset:", err) | ||
| } |
| DispatchQueue.main.async { | ||
| guard let self else { return } | ||
| self.installedRulesRunning = false | ||
| if let decoded { | ||
| self.installedRules = decoded |
| validate Load and validate a config file (no root, no side effects) | ||
| monitor Live read-only view: IP, country, tunnel state, endpoints, verdict | ||
| print-rules Print the firewall ruleset a block/guard would apply, without applying it | ||
| print-rules Print the firewall ruleset a block/guard would apply (--applied: what is applied now) |
Round 2. The headline finding is the loop's own: round 1 taught the no-rules branch to print the readback text, because on Windows the blocking lives in each profile's DefaultOutboundAction rather than in the rule group — and left the DRIFT branch, eight lines above, still throwing it away. That is the branch taken whenever a record exists, which is exactly when someone is asking. A host whose group was removed while its profile default is still Block is fully cut, and was told "the kernel holds no dezhban rules" with the profile table that proves egress is cut discarded. Both branches print it now. From the branch itself: - pf checked two of the three things IsBlocked checks. An anchor that is loaded and referenced while pf is switched off entirely (`pfctl -d`) filters nothing, and rendered as a clean readback with no warning. The status probe is now there, in the same shape as the anchor-reference verdict, and says so. - The whole Firewall-rules section sat inside the pane's `doctorReport != nil || vpnInventory != nil` gate, so the applied record and the "Read from the kernel…" button were invisible on a host where `doctor --json` cannot run — the state someone is most likely diagnosing — and on every first open until the async doctor returned. None of the three rows needs doctor. This is the same bug the comment right above that gate describes for the VPN inventory, one section over. - The applied row put the posture in its title and the time only in its caption, so a pane held open across GUARD → FULL BLOCK kept reading "Applied by dezhban — Guard". The time is now in the title, where the claim is made. That read is unprivileged and cheap, which is why it gets a timestamp rather than the clearing the kernel row got. - `--config` was refused but not documented; the help line named `--applied` but not `--installed`; and all three completion scripts offered neither. The wiring, not just the helpers, is now tested. Both fixes this loop made could be deleted with the whole suite still green: TestEveryDirectFirewallPathKeepsTheRecordHonest walks main.go's AST and fails when cmdBlock stops recording or cmdPanic/cmdUnblock stop clearing (an AST guard because all three need root and a real firewall — same technique, and same reason, as TestNoTestInPackageMainIsParallel), and TestRunWiresTheRecordingBackend drives Run end to end and fails with "recorded nothing — the backend was never wrapped" when the decorator is unwired. Both were confirmed against the unfixed code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Diagnostics gains a Firewall rules section with three sources, because they answer three different questions and are not interchangeable:
Each carries a plain-language caption saying what that posture does to your traffic — a ruleset is not self-explanatory to the person most likely to be reading it.
Recording what was applied
internal/appliedwrites the exact ruleset text handed to the backend, timestamped, besidestate.jsonat 0644 like the state file — so the unprivileged menubar app can read it without root. It holds nothingprint-ruleswould not print for free.Recorded by wrapping the runner's
Backend, not by addingapplied.Savebeside eachApply. The run loop applies from nineteen places, and a record that is only as complete as the last person to remember it is worse than none — a surface would show a stale posture with no way to tell. Wrapping makes a new call site recorded by construction.The wrapper adds no goroutine and no writer: every method is called from the run-loop goroutine by the same code that called the backend before, so CLAUDE.md's single-writer invariant is untouched. The write is an atomic replace of a small file — bounded work on the goroutine that owns window expiry and geo ticks, which is why it must stay that shape.
Two properties worth calling out:
Apply. A failed apply leaves the previous ruleset live, so recording the attempt would describe rules that were never installed — the one thing a reader of this file must be able to rely on not happening.UnblockandCleanupclear it, even when they fail. A record surviving teardown would be read as the live posture: a pane saying "guard is enforcing" over a wide-open network.It wraps
runner.Backend(the narrow enforcement interface), notfirewall.FirewallBackend, so the diagnostic read below does not end up on the seam enforcement uses.Reading the kernel back
FirewallBackendgainsInstalledRules() (string, bool, error), implemented for all three backends:pfctl -a dezhban -s rules, plus a warning line when the main ruleset no longer references the anchor (loaded but never descended into — the same gapIsBlockedchecks).listTable, plus a warning when the output chain's policy has drifted offdrop.Get-NetFirewallRule -Group dezhbanplus each profile'sDefaultOutboundAction, since on Windows that is where the blocking actually lives.Every one is scoped to dezhban's own anchor/table/group, so this can never become a way to dump a user's unrelated firewall configuration. It is a read: it does not go through
Applyand does not touch the single-writer rule, so any goroutine or process may call it. It needs root, which is why nothing calls it on a tick and the daemon never calls it at all.Drift is reported, never repaired
When dezhban has a record of applying rules and the kernel holds none, both the CLI and the pane say so — and offer no repair. The run loop's
VerifyIntervaltick already re-applies missing rules; a repair button would be a second writer of the firewall.Neither surface diffs the two texts.
pfctl -s rulesrenders a normalised form of what was loaded, so a byte comparison would report drift on every healthy host. The texts are shown for a person to read, and thedriftflag is the narrow, reliable signal.CLI
--jsonon either. Passing both is refused with an explanation, because they are two different sources rather than two views of one. Without root,--installedfails with thesudohint rather than a bare permission error."Nothing recorded" exits 0, not 1 — a daemon in standby has applied nothing, and that must be distinguishable from a failure.
Also
The Diagnostics previews are lazy: expanding a posture spawns its
print-rulessubprocess, collapsed ones spawn nothing. Rendering all three on every visit to the pane would be three processes nobody asked for.Verification
go build,go vet,go test— pass, includingGOOS=linuxandGOOS=windowsvet for the two backends this machine cannot run.internal/appliedtests: round trip, 0644 (the GUI has to read it), missing-is-not-an-error,Removeidempotent, corrupt-is-discarded-not-fatal.internal/runnertests: what gets recorded matchesRenderRulesfor the same policy, a failed apply leaves the previous record intact,Unblock/Cleanupclear it, an empty path returns the backend unwrapped, and a nil logger does not panic (Runnever defaultsLog).swift test— 205 tests.RulesetsTestscovers Go's RFC 3339 fractional timestamps, which Foundation's.iso8601strategy rejects outright — that would have turned a good record into "no rules recorded" while the guard was enforcing.print-rules --appliedand--installedexercised directly; the unprivileged--installedpath produces the intended refusal and hint.build-app.shassembles cleanly.The parts CI cannot reach are in docs/contribute/testing.md under a new "Firewall rules (Diagnostics)" section — most importantly: teardown clears the record, the readback changes nothing, and flushing the anchor by hand produces a warning with no repair button while the daemon's own verify tick heals it.
🤖 Generated with Claude Code